Conversation
…plate code reduction
…r and match handling
[Fix]:Refactor CompetitionTable and Round classes to improvemets
…and referee handling logic
…r and improve relationship management
…ree management logic
|
Hi @xji650, this PR is not linked to any issue. |
udl-softarch
bot
added
the
pr-missing-linked-issue
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR refactors four domain entities to use Lombok-based boilerplate generation, replacing explicit getter/setter methods with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There was a problem hiding this comment.
Pull request overview
This PR refactors several JPA domain entities to reduce boilerplate by adopting Lombok-generated accessors/constructors and adjusts entity relationship helper methods to better manage bidirectional associations (including null/duplicate handling).
Changes:
- Replace explicit constructors/getters/setters with Lombok annotations (
@Getter,@Setter,@NoArgsConstructor) across multiple entities. - Add/adjust
equals/hashCodegeneration via Lombok, primarily including entity identifiers. - Update
add/removehelpers forRoundandCompetitionTableto handle nulls, de-duplication, and moving children between parents; adjust cascade settings for table→matches.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/main/java/cat/udl/eps/softarch/fll/domain/Round.java |
Lombok refactor + updated match association helpers |
src/main/java/cat/udl/eps/softarch/fll/domain/Referee.java |
Lombok refactor + Lombok equals/hashCode configuration |
src/main/java/cat/udl/eps/softarch/fll/domain/Match.java |
Lombok refactor + newly introduced equals/hashCode on generated id |
src/main/java/cat/udl/eps/softarch/fll/domain/CompetitionTable.java |
Lombok refactor + updated match/referee association helpers + cascade change |
| public void addMatch(Match match) { | ||
| if (match == null) { | ||
| return; | ||
| } | ||
|
|
||
| if (matches.contains(match)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
addMatch uses matches.contains(match) to prevent duplicates, but Match now has Lombok equals/hashCode based only on the generated id. For transient matches (where id == null), all instances compare equal, so adding multiple new Match() objects will be incorrectly blocked (and removeMatch may remove the wrong instance). Consider changing the duplicate check to use object identity when id is null, or revising Match.equals/hashCode so entities with null ids are not considered equal.
| @Getter | ||
| @Setter | ||
| @NoArgsConstructor | ||
| @EqualsAndHashCode(callSuper = false, onlyExplicitlyIncluded = true) | ||
| public class Match extends UriEntity<Long> { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| @EqualsAndHashCode.Include | ||
| private Long id; |
There was a problem hiding this comment.
@EqualsAndHashCode includes only the generated id. When id is null (common before persistence), different Match instances will compare equal and share the same hash code. This can break collection operations (e.g., List.contains/remove) and is currently observable via Round.addMatch/CompetitionTable.addMatch duplicate checks. Consider implementing an equality strategy that does not treat two transient entities as equal (e.g., only compare ids when both are non-null).
| public void addMatch(Match match) { | ||
| if (match == null || matches.contains(match)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
addMatch relies on matches.contains(match) for de-duplication, but Match.equals/hashCode is based solely on a generated id. This makes all transient matches (id == null) appear equal, so adding multiple new matches to a table can be silently skipped and removeMatch may remove an unintended instance. Use an identity-based duplicate check for transient entities or adjust Match.equals/hashCode to avoid null-id equality.
| public void addReferee(Referee referee) { | ||
| if (referee == null || referees.contains(referee)) { |
There was a problem hiding this comment.
addReferee uses referees.contains(referee) to prevent duplicates. Since Referee equality comes from Volunteer and is based on a generated id, two new Referee() instances (with id == null) will compare equal and the second one will be dropped. If you need duplicate prevention, consider checking duplicates by object identity when id is null (or only by id when non-null).
| public void addReferee(Referee referee) { | |
| if (referee == null || referees.contains(referee)) { | |
| private boolean isRefereeAlreadyAssigned(Referee referee) { | |
| if (referee == null) { | |
| return false; | |
| } | |
| Object refereeId = referee.getId(); | |
| if (refereeId == null) { | |
| for (Referee existingReferee : referees) { | |
| if (existingReferee == referee) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| for (Referee existingReferee : referees) { | |
| if (refereeId.equals(existingReferee.getId())) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| public void addReferee(Referee referee) { | |
| if (referee == null || isRefereeAlreadyAssigned(referee)) { |
| @@ -58,12 +45,30 @@ public void setMatches(List<Match> matches) { | |||
| } | |||
There was a problem hiding this comment.
setMatches clears this.matches without updating the owning side (Match.round). This can leave previously associated Match instances still pointing to this Round, breaking bidirectional consistency and potentially confusing JPA orphan removal. Prefer iterating over a copy and calling removeMatch for each existing match before adding the new ones.
No description provided.